[CompilerPlugin] Remove Foundation dependency #2598

rintaro · 2024-04-11T22:43:31Z

read(2) / write(2) instead of Foundation.FileHandle
Implement its own JSON encoder/decoder

rdar://126714084

bnbarham · 2024-04-12T16:39:06Z

@swift-ci Please test Windows

rintaro · 2024-04-12T22:01:26Z

swiftlang/swift#73010
@swift-ci Please test Windows

Inspired by swift-foundation's JSONEncoder/JSONDecoder. But basically a re-implementation of Encoder / Decoder.

Instead, use read(2) and write(2) via Darin/Glibc/UCRT

* Rename PluginMessageJSON to JSON * Make JSON encoder/decoder SPI * Make encoding/decoding internals private

Use DecodingError / EcodingError properly

If read(2) returned 0, STDIN is closed. So return nil from 'waitForNextMessage()' so the caller can exit cleanly.

Add _CShims module. For CMake it's a INTERFACE library.

Speculative fix for Windows

Sources/SwiftCompilerPlugin/CompilerPlugin.swift

Sources/SwiftCompilerPluginMessageHandling/JSON/JSONDecoding.swift

Sources/SwiftCompilerPluginMessageHandling/JSON/JSONEncoding.swift

rintaro · 2024-04-19T20:12:11Z

Sources/SwiftCompilerPluginMessageHandling/JSON/JSONDecoding.swift

+  mutating func closeCollection(handle: Int) {
+    // 'handle': descriptor index.
+    // 'handle+1': counter index.
+    mapData[handle + 1] = mapData.count - handle - 2


There's no reason to - 2 here. Only usage (i.e. JSONMapValue.mapSize) is doing + 2. Let's just store the mapSize value, so mapSize can return the stored value as-is.

There's no reason to '- 2'/'+ 2'.

* typo * Remove unnecessary withExtendedLifetime() * remove JSONObject.subscript(_:) as it's not O(1) * Use 'LosslessStringConvertible'

* Don't use UTF8 decoder for decoding escaped strings * Only memcmp ASCII non-escaped strings

ahoppen

Looks good in general. Don’t be distracted by the truckload of comments that I’m leaving here. I think most are nitpicky, just a few correctness questions.

Package.swift

Sources/SwiftCompilerPlugin/CompilerPlugin.swift

Sources/SwiftCompilerPluginMessageHandling/JSON/JSONEncoding.swift

Update for review comments

rintaro · 2024-04-21T13:29:52Z

@swift-ci Please test

rintaro · 2024-04-21T15:08:48Z

@swift-ci Please test

ahoppen · 2024-04-22T18:55:52Z

Sources/SwiftCompilerPluginMessageHandling/JSON/JSONEncoding.swift

+    guard case .array(var arr) = backing else {
+      preconditionFailure()
+    }
+    backing = .null  // Ensure 'err' uniquely referenced.


Nitpick 😉

Suggested change

backing = .null // Ensure 'err' uniquely referenced.

backing = .null // Ensure 'arr' uniquely referenced.

Sources/SwiftCompilerPluginMessageHandling/JSON/JSONEncoding.swift

ahoppen

Only open discussion is why we needed to define compareMemory for SyntaxText but can unconditionally use memcmp here: #2598 (comment)

Update for review comments

Use 'for' look instead of 'while cursor != end { cursor += 1 }'

The source JSON buffer is not guaranteed to be null terminated. If a number is at top-level, 'strtod'/'strtof' tries to read past the buffer which is not great

Avoid possible module name collision

rintaro · 2024-04-22T19:11:19Z

@swift-ci Please test

rintaro · 2024-04-22T19:12:12Z

@swift-ci Please test Windows

rintaro · 2024-04-22T20:11:45Z

@swift-ci Please test Windows

rintaro mentioned this pull request Apr 12, 2024

[Macros] Use SwiftCompilerPluginMessageHandling JSON encoder/decoder swiftlang/swift#72999

Merged

rintaro force-pushed the macros-jsoncoder branch 2 times, most recently from 38e6d49 to 7fbf536 Compare April 12, 2024 17:34

rintaro force-pushed the macros-jsoncoder branch from ed1384b to ccc6f9b Compare April 12, 2024 23:58

rintaro mentioned this pull request Apr 13, 2024

DO NOT MERGE : [Windows] Test building swift-syntax/Examples swiftlang/swift#73021

Draft

rintaro force-pushed the macros-jsoncoder branch from abe880f to 694c846 Compare April 15, 2024 03:52

rintaro added 19 commits April 16, 2024 14:59

[Macros] Implement own JSON encoder/decoder

61c7338

Inspired by swift-foundation's JSONEncoder/JSONDecoder. But basically a re-implementation of Encoder / Decoder.

[CompilerPlugin] Remove dependency to Foundation.FileHandle

e20a65c

Instead, use read(2) and write(2) via Darin/Glibc/UCRT

[Macros] Cleanup JSON

29032ed

* Rename PluginMessageJSON to JSON * Make JSON encoder/decoder SPI * Make encoding/decoding internals private

[Macros] Update CMakeLists.txt for JSON

7886469

[JSON] Improve error throwing

3d676d0

Use DecodingError / EcodingError properly

[JSON] Formatting

39acd5a

[JSON] Import for non-Darwin platforms

c830a21

[CompilerPlugin] Exit cleanly

adb2fb0

If read(2) returned 0, STDIN is closed. So return nil from 'waitForNextMessage()' so the caller can exit cleanly.

[JSON] Comment and cosmetic change

4edb04a

[JSON] Use only inside of string quote

b8ae7a6

[JSON] Add basic test cases

d6f5327

[JSON] Encode/decode escape sequence correctly

9246250

[JSON] Misc improvements

c986761

[JSON] Optimization and tweaks

6b594b8

[CompilerPlugin] Add C shim for getting stdio names and errno

10cc964

Add _CShims module. For CMake it's a INTERFACE library.

[CompilerPlugin] Use fread(3)/fwrite(3) instead of read(2)/write(2)

3b36bde

Speculative fix for Windows

[Windows] Speculative fix

b3494aa

[JSON] Fix UnkeyedContaier.decodeNil()

585dae0

[JSON] Emit DecodingError.valueNotFound error if the value is null

5250447

rintaro force-pushed the macros-jsoncoder branch from 8ffdc15 to 5250447 Compare April 16, 2024 22:01

[JSON] expect(_:) should throw 'unexpectedEndOfFile' at EOF

27fc293

rintaro force-pushed the macros-jsoncoder branch from cd01261 to 501f454 Compare April 18, 2024 07:37

rintaro marked this pull request as ready for review April 19, 2024 13:56

rintaro requested review from ahoppen and bnbarham as code owners April 19, 2024 13:56

rintaro requested a review from hamishknight April 19, 2024 14:20

hamishknight approved these changes Apr 19, 2024

View reviewed changes

rintaro commented Apr 19, 2024

View reviewed changes

rintaro added 3 commits April 19, 2024 19:06

[JSON] Collection values in JSONMap to store the mapSize

c0fc9b5

There's no reason to '- 2'/'+ 2'.

[JSON] Small changes

4c84edb

* typo * Remove unnecessary withExtendedLifetime() * remove JSONObject.subscript(_:) as it's not O(1) * Use 'LosslessStringConvertible'

[JSON] Improve string decoding

5ab0c7e

* Don't use UTF8 decoder for decoding escaped strings * Only memcmp ASCII non-escaped strings

ahoppen reviewed Apr 20, 2024

View reviewed changes

[JSON] Style and documentation improvements, and cleanups

197a45c

Update for review comments

rintaro force-pushed the macros-jsoncoder branch from 7d7902c to 197a45c Compare April 21, 2024 13:01

rintaro force-pushed the macros-jsoncoder branch 3 times, most recently from e02a7a0 to a684b3b Compare April 22, 2024 18:56

ahoppen reviewed Apr 22, 2024

View reviewed changes

ahoppen approved these changes Apr 22, 2024

View reviewed changes

rintaro added 4 commits April 22, 2024 12:05

[JSON] Style and documentation improvements, and cleanups

17a0c6b

Update for review comments

[JSON] JSONWriter.serialize(string:) improvement

cf4f3e4

Use 'for' look instead of 'while cursor != end { cursor += 1 }'

[JSON] Fix a potential buffer over read in floating point parsing.

679b929

The source JSON buffer is not guaranteed to be null terminated. If a number is at top-level, 'strtod'/'strtof' tries to read past the buffer which is not great

Rename _CShims -> _SwiftSyntaxCShims

1e2c3bd

Avoid possible module name collision

rintaro force-pushed the macros-jsoncoder branch from a684b3b to 1e2c3bd Compare April 22, 2024 19:05

hamishknight approved these changes Apr 22, 2024

View reviewed changes

rintaro enabled auto-merge April 22, 2024 20:12

rintaro merged commit 462e954 into swiftlang:main Apr 22, 2024
3 checks passed

	backing = .null // Ensure 'err' uniquely referenced.
	backing = .null // Ensure 'arr' uniquely referenced.

[CompilerPlugin] Remove Foundation dependency #2598

[CompilerPlugin] Remove Foundation dependency #2598

Uh oh!

Conversation

rintaro commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnbarham commented Apr 12, 2024

Uh oh!

rintaro commented Apr 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rintaro Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rintaro commented Apr 21, 2024

Uh oh!

rintaro commented Apr 21, 2024

Uh oh!

ahoppen Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

rintaro commented Apr 22, 2024

Uh oh!

rintaro commented Apr 22, 2024

Uh oh!

rintaro commented Apr 22, 2024

Uh oh!

Uh oh!

Uh oh!

rintaro commented Apr 11, 2024 •

edited

Loading